[Docs] Templates - Ascending navigation update#5653
Conversation
Added interactive example file. Updated page copy & structure. Updated copy on pageheading/card via templates.js
Updated example file. Update written guiguidance on Julias feedback
|
✅ Deploy Preview for hpe-theme-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hpe-design-icons-grommet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for unrivaled-bublanina-3a9bae ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
julauxen
left a comment
There was a problem hiding this comment.
Looks good on my side. Ready for devs to review
…cs-ascending-navigation-update
apps/docs/src/examples/templates/ascending-navigation/AscendingNavigationAnatomy.js
Show resolved
Hide resolved
aries-site/src/examples/templates/ascending-navigation/AscendingNavigationExample.js
Outdated
Show resolved
Hide resolved
aries-site/src/examples/templates/ascending-navigation/AscendingNavigationExample.js
Outdated
Show resolved
Hide resolved
|
@SeamusLeonardHPE I rearranged some code to be a little cleaner. I will be off Monday however I was thinking if @sulaymon-tajudeen-hpe also has time to walk you through some of the changes if not we can go through next week. |
A card with an arrow is used in some real partner apps. Is that the question? |
yes it was a question. I guess I have not seen this use of Card even on our Card page example so wasnt sure. |
aries-site/src/examples/templates/ascending-navigation/AscendingNavigationExample.js
Outdated
Show resolved
Hide resolved
| <Text weight="bold">To create this demo application:</Text> | ||
| <Paragraph margin="none">1. Create a React sandbox.</Paragraph> | ||
| <Paragraph margin="none"> | ||
| 2. Install <code>grommet</code>, <code>grommet-theme-hpe</code>,{' '} | ||
| <code>@hpe-design/icons-grommet</code>. | ||
| </Paragraph> |
There was a problem hiding this comment.
Same problem here (e.g. <ol><li> or Markdown would be better.) I wondering if this is important to help the screen readers
aries-site/src/examples/templates/ascending-navigation/index.js
Outdated
Show resolved
Hide resolved
@britt6612 Being used in Cillian's explorations of page templates |
Added new line to Index Placed example content inside markdown. Removed Nav in header (feedback form show & tell)
|
@halocline tagging to request final review. Thanks |
halocline
left a comment
There was a problem hiding this comment.
The end result of the example is quite nice. That said, we need to clean up the execution for how we achieve that result. I'd like to reduce implementation complexity while increasing readability, maintainability, and keep and mind that we are serving as reference examples -- people will copy and paste whatever we do.
| <PageHeader | ||
| parent={ <Anchor label="Home" icon={<Left />} a11yTitle="Link to the home page" />} | ||
| title="Example page title" | ||
| subtitle="I'm a direct child of the homepage" | ||
| /> |
There was a problem hiding this comment.
There was a problem hiding this comment.
Will need to connect with @britt6612 to help me learn how to add raw code urls via code prop
| <AscendingNavigationExample /> | ||
| </Example> | ||
|
|
||
| ```txt |
There was a problem hiding this comment.
If we are going to display this code sample inline, this should be jsx not txt.
| ```txt | |
| ```jsx |
There was a problem hiding this comment.
Updated to jsx
| 1. A preview of Users detail view (child page). | ||
| 1. Page loads on <strong>Overview</strong> child page | ||
| </Text> | ||
| <br></br> |
There was a problem hiding this comment.
Are we using caption (which renders in the DOM as a figcaption) appropriately here? It seems like we are trying to describe the experience/interaction behavior vs. the figcaption "describing the rest of the contents of its parent
I'd expect a figcaption to read "Example demonstrating how an "ascending navigation" anchor allows a user to navigate from a child page to its parent."
I'm not sure the "1. Page loads on Overview child page" and "2. Interacting with the Ascending Navigation link brings the user to the home page" adds much value -- can likely be eliminated. If we do think describing these interactions is needed, this should probably be placed in line with the preceding paragraph.
| }; | ||
|
|
||
| return ( | ||
| <Grommet theme={hpe}> |
There was a problem hiding this comment.
We should not have a Grommet wrapper here.
| <Grommet theme={hpe}> |
| <Box | ||
| background="background-back" | ||
| width="full" | ||
| border={{ color: 'border-weak', size: 'xsmall' }} | ||
| pad={{ bottom: 'xlarge' }} | ||
| > |
There was a problem hiding this comment.
We are re-creating how "Example containers" are displayed within the site and creating fragmentation, additional deviation and overhead. Why not just use the Example container's properties for consistency?
| <Box | |
| background="background-back" | |
| width="full" | |
| border={{ color: 'border-weak', size: 'xsmall' }} | |
| pad={{ bottom: 'xlarge' }} | |
| > | |
| <> |
There was a problem hiding this comment.
Replaced custom box container with example.
| > | ||
| <Button | ||
| default | ||
| icon={<Home />} |
There was a problem hiding this comment.
The screen reader will read out both "Go to Home page" and the icon's aria-label "Home". Which is redundant and confusing. Need to suppress the latter.
| icon={<Home />} | |
| icon={<Home aria-hidden="true" />} |
| default | ||
| icon={<Home />} | ||
| onClick={() => setActiveSection('home')} | ||
| hoverIndicator="background" |
There was a problem hiding this comment.
This is unnecessary.
| hoverIndicator="background" |
| import { sectionConfig } from './data/sections'; | ||
|
|
||
| export const AscendingNavigationExample = () => { | ||
| const [activeSection, setActiveSection] = useState('overview'); |
There was a problem hiding this comment.
These represent pages, not sections. For clarity variable names should align to what they represent.
Comment applies to references of "section" throughout.
| const [activeSection, setActiveSection] = useState('overview'); | |
| const [activePage, setActivePage] = useState('overview'); |
| export const AscendingNavigationExample = () => { | ||
| const [activeSection, setActiveSection] = useState('overview'); | ||
|
|
||
| const config = sectionConfig[activeSection]; |
There was a problem hiding this comment.
In this a configuration or the pages content?
| const config = sectionConfig[activeSection]; | |
| const page = pageContent[activePage]; |
| @@ -0,0 +1,17 @@ | |||
| import { Announce, Help, Template } from '@hpe-design/icons-grommet'; | |||
There was a problem hiding this comment.
I'm not really a fan of this pattern here because it is harder to read and takes a lot of effort to follow. For clarity and flexibility, I'd rather follow the pattern you use in HomeContent.js. Similarly create OverviewContent.js, Components.js, Help.js and eliminate the whole config concept.
Instead you can replace all of that with:
const [activePage, setActivePage] = useContext('home');
const pageContent = {
home: <HomeContent />,
overview: <OverviewContent />,
components: <ComponentsContent />,
help: <HelpContent />
};
....
<>
{pageContent[activePage]}
<>
There was a problem hiding this comment.
Moved into separate pages and named PageOne, PageTwo... etc so we can update to any content.
julauxen
left a comment
There was a problem hiding this comment.
Visuals and content are looking good. Ready for dev review
…cs-ascending-navigation-update
…/hpe-design-system into docs-ascending-navigation-update
aries-site/src/examples/templates/ascending-navigation/components/PageOneContent.js
Outdated
Show resolved
Hide resolved
apps/docs/src/examples/templates/ascending-navigation/components/PageTwoContent.js
Outdated
Show resolved
Hide resolved
| render: datum => ( | ||
| <Box direction="row" gap="small" align="center"> | ||
| <Avatar background="brand" size="small"> | ||
| {datum.initials} |
There was a problem hiding this comment.
the initials wont pass accessibility we need to use
<Text size="large" color="text-strong">
JS
</Text>
There was a problem hiding this comment.
Changed the BG color and font size, passed acessibiity test in chrome
<Avatar background="dark-2" size="small"> <Text size="medium" color="text-strong">{datum.initials}</Text> </Avatar>
There was a problem hiding this comment.
@SeamusLeonardHPE sorry but can we use background="decorative-green" to match our other examples for Avatar and I think we are recommending using decorative-green
There was a problem hiding this comment.
Updated to
1- color decorative-green
2- Size medium
3- Removed text size prop (inherited from avatar)
<Avatar background="decorative-green" size="medium">
<Text color="text-strong">
{datum.initials}
</Text>
</Avatar>
| <Heading | ||
| level="2" | ||
| size='small' | ||
| margin={{ |
There was a problem hiding this comment.
why do we need this margin?
| > | ||
| {section.label} | ||
| </Heading> | ||
| <LinkNext height="xxlarge" /> |
There was a problem hiding this comment.
I dont think we need the height
There was a problem hiding this comment.
Aligns icon height with heading line height using technique suggested in guidance
https://design-system.hpe.design/foundation/icons#aligning-with-text
apps/docs/src/examples/templates/ascending-navigation/components/SectionCard.js
Outdated
Show resolved
Hide resolved
|
|
||
| <Paragraph alignSelf="top">{section.subtitle}</Paragraph> | ||
| </Card> | ||
| ); |
There was a problem hiding this comment.
can we simplify code to be
export const SectionCard = ({ section, onClick }) => (
<Card default onClick={() => onClick(section.id)} pad="xsmall">
<Box align="center" direction="row" fill justify="between">
<Heading level="2" size="small">
{section.label}
</Heading>
<LinkNext />
</Box>
<Paragraph>{section.subtitle}</Paragraph>
</Card>
);
There was a problem hiding this comment.
Adressed need for addtional props in points above
export const SectionCard = ({ section, onClick }) => (
<Card default onClick={() => onClick(section.id)} pad="xsmall">
<Box direction="row" fill justify="between">
<Heading level="2" size="medium" margin="none">
{section.label}
</Heading>
<LinkNext height="xlarge" />
</Box>
<Paragraph>{section.subtitle}</Paragraph>
</Card>
);
apps/docs/src/examples/templates/ascending-navigation/components/NavigationButton.js
Outdated
Show resolved
Hide resolved
|
@britt6612 added responses into all comments, if you can review modified code and resolve if deemed suitable. |
apps/docs/src/examples/templates/ascending-navigation/components/SectionCard.js
Outdated
Show resolved
Hide resolved
| const [selected, setSelected] = useState([]); | ||
|
|
||
| return ( | ||
| <Box> |
There was a problem hiding this comment.
| <Box> |
I dont think we need those boxes
| const [selected, setSelected] = useState([]); | ||
|
|
||
| return ( | ||
| <Box> |
There was a problem hiding this comment.
remove this box as well





https://deploy-preview-5653--keen-mayer-a86c8b.netlify.app/templates/ascending-navigation
What does this PR do?
replaces 2 static image examples with an interactive demo of ascending navigation functionality
What are the relevant issues?
Closes #5725
Files modified
templates.js
Updated description to for clarity and to avoid duplication of phrasing on the page
AscendingNavigationAnatomy.js
Removed
AscendingNavigationExample.js
Added demo with interactions between page & parent link
ascending-navigation.mdx
Note that during the site rebuild i think this content should be moved into the PageHeader as a configuration section.